-
Notifications
You must be signed in to change notification settings - Fork 694
Import gvisor-tap-vsock/pkg/tcpproxy
into pkg/tcpproxy
#3686
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
a3eb1bb
to
f946103
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍 Thanks for the updates
Failures on WSL2 🤔
|
proxy := tcpproxy.DialProxy{DialContext: func(_ context.Context, _, _ string) (net.Conn, error) { | ||
return conn, nil | ||
}} | ||
proxy.HandleConn(rw) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unlike bicopy.Bicopy(), this ignores errors silently. I don't think this library should be used for anything in the current state.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What should we do then ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Port forwarding seems to be lima core functionality, and using a library makes it hard to get good logging, so It think the best way is to copy the missing bits from tcpproxy into our own implementation.
This may be related:
inetaf/tcpproxy#46
tcpproxy seems to be about
Package tcpproxy lets users build TCP proxies, optionally making routing decisions based on HTTP/1 Host headers and the SNI hostname in TLS connections.
Our usage seems to be hacky way to reuse part of the library.
It also says:
This package makes no API stability promises. If you depend on it, vendor it.
Other options:
- Log errors in the wrappers we pass to tcpproxy
- Fix tcpproxy to log errors properly
If nobody can own this code in lima we can use tcpproxy as a temporary quick fix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Port forwarding seems to be lima core functionality, and using a library makes it hard to get good logging, so It think the best way is to copy the missing bits from tcpproxy into our own implementation.
I have no idea how big tcpproxy is, but we have copied single file implementations from other projects before:
❯ ag adapted
pkg/hostagent/dns/dns.go
4:// This file has been adapted from https://github.com/norouter/norouter/blob/v0.6.4/pkg/agent/dns/dns.go
pkg/reflectutil/reflectutil.go
4:// This file has been adapted from https://github.com/containerd/nerdctl/blob/v1.0.0/pkg/reflectutil/reflectutil.go
So assuming the implementation isn't too big, I think importing it gives us the maximum flexibility going forward.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Port forwarding seems to be lima core functionality, and using a library makes it hard to get good logging, so It think the best way is to copy the missing bits from tcpproxy into our own implementation.
I have no idea how big tcpproxy is
tcpproxy is pretty small, and we basically use one function:
https://github.com/inetaf/tcpproxy/blob/c4b9df066048ad2ab5c32235362fa94444a24ebe/tcpproxy.go#L376
func (dp *DialProxy) HandleConn(src net.Conn) {
ctx := context.Background()
var cancel context.CancelFunc
if dp.DialTimeout >= 0 {
ctx, cancel = context.WithTimeout(ctx, dp.dialTimeout())
}
dst, err := dp.dialContext()(ctx, "tcp", dp.Addr)
if cancel != nil {
cancel()
}
if err != nil {
dp.onDialError()(src, err)
return
}
defer dst.Close()
if err = dp.sendProxyHeader(dst, src); err != nil {
dp.onDialError()(src, err)
return
}
defer src.Close()
if ka := dp.keepAlivePeriod(); ka > 0 {
for _, c := range []net.Conn{src, dst} {
if c, ok := tcpConn(c); ok {
c.SetKeepAlive(true)
c.SetKeepAlivePeriod(ka)
}
}
}
errc := make(chan error, 2)
go proxyCopy(errc, src, dst)
go proxyCopy(errc, dst, src)
<-errc
<-errc
}
In this file we use it to proxy packets between GRPC endpoint and net.Con - both are connected, so we hack the proxy DialContext function to not dial anything.
We don't need the sendProxyHeader() thing since we don't to the optional http proxy feature.
I'm not sure if the keepalive is needed for our use case, but GRPC is not a net.TCPCon so it does nothing for it.
So we are left with:
defer dst.Close()
defer src.Close()
if ka := dp.keepAlivePeriod(); ka > 0 {
for _, c := range []net.Conn{src, dst} {
if c, ok := tcpConn(c); ok {
c.SetKeepAlive(true)
c.SetKeepAlivePeriod(ka)
}
}
}
errc := make(chan error, 2)
go proxyCopy(errc, src, dst)
go proxyCopy(errc, dst, src)
<-errc
<-errc
Note how the errors are dropped silently to make the user life more interesting.
proxyCopy is:
func proxyCopy(errc chan<- error, dst, src net.Conn) {
defer closeRead(src)
defer closeWrite(dst)
// Before we unwrap src and/or dst, copy any buffered data.
if wc, ok := src.(*Conn); ok && len(wc.Peeked) > 0 {
if _, err := dst.Write(wc.Peeked); err != nil {
errc <- err
return
}
wc.Peeked = nil
}
// Unwrap the src and dst from *Conn to *net.TCPConn so Go
// 1.11's splice optimization kicks in.
src = UnderlyingConn(src)
dst = UnderlyingConn(dst)
_, err := io.Copy(dst, src)
errc <- err
}
Since we don't use tcpproxy.Con, and we don't do route matching we don't need to copy buffered data.
Since GRPC endpoint is not a net.TCPConn unwrapping the underlying connection will fail, and the splice optimization will not kick in.
So we are left with:
func proxyCopy(errc chan<- error, dst, src net.Conn) {
defer closeRead(src)
defer closeWrite(dst)
_, err := io.Copy(dst, src)
errc <- err
}
I'm not sure how this code supports TCP half-close and bicopy does not. They seem to do the same thing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The issue might not be relevant to tcp half-close , as this does not seem to implement half-close methods
lima/pkg/portfwdserver/server.go
Line 42 in 3bc63b6
type GRPCServerRW struct { |
Not sure why tcpproxy
fixes the issue, except on WSL2 🤔
gvisor-tap-vsock/pkg/tcpproxy
into pkg/tcpproxy
8db0dbe
to
6e67594
Compare
Import https://github.com/containers/gvisor-tap-vsock/blob/v0.8.6/pkg/tcpproxy/tcpproxy.go into `pkg/tcpproxy`. Changes to the file will be added in follow-up commits. Fix issue 3685 Signed-off-by: Akihiro Suda <[email protected]>
Signed-off-by: Akihiro Suda <[email protected]>
Signed-off-by: Akihiro Suda <[email protected]>
Signed-off-by: Akihiro Suda <[email protected]>
Fix #3685
Similar to: